Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add possbility to override the max size fo groups #6229

Closed
wants to merge 1 commit into from

Conversation

kisieland
Copy link
Contributor

This allowes for more control over for the CA scale-up simulation.

What type of PR is this?

What this PR does / why we need it:

Allows users to control the size of the pods per group in the simulation.
This will help in cases where multiple similar pods are being scheduled.
The original hard-coded value of 10 was limiting in cases where there are 1000 of similar pods being created at once.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?


Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


This allowes for more control over for the CA scale-up simulation.
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 27, 2023
@k8s-ci-robot k8s-ci-robot requested a review from x13n October 27, 2023 10:40
@jayantjain93
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jayantjain93, kisieland
Once this PR has been reviewed and has the lgtm label, please assign bigdarkclown for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@@ -76,7 +74,7 @@ func groupPodsBySchedulingProperties(pods []*apiv1.Pod) map[equivalenceGroupId][
podEquivalenceGroups[*gid] = append(podEquivalenceGroups[*gid], pod)
continue
}
if len(egs) < maxEquivalenceGroupsByController {
if len(egs) < maxPodsPerGroup {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, this used to be # of equivalence groups per controller and now you're suddenly using max pods per group here. What problem are you trying to solve?

@x13n
Copy link
Member

x13n commented Nov 2, 2023

1000 similar pods should only result in one pod equivalence group. If there's more groups, it means they are not similar. Maybe you're hitting a problem because of some criterium we should add to the pod equivalence comparison?

@kisieland kisieland closed this Nov 2, 2023
@kisieland
Copy link
Contributor Author

Thanks @x13n, the pods were missing the Controller flag to be grouped.

@kisieland kisieland deleted the bigger-grouping branch November 2, 2023 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cluster-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants